fix(watch): enforce ClusterProvider admission wherever rules compile#258
Conversation
A ClusterWatchRule resolved its GitTarget with a plain Get and no authorization check, so it could attach to a GitTarget in any namespace and widen that target's mirror scope cluster-wide without the compilation path ever consulting the ClusterProvider's allowedNamespaces policy for that target's namespace. This was not a live escalation — ClusterWatchRule is cluster-scoped, and the gate held transitively because checkSourceAuthorization returns before DeclareForGitTarget, so no targetWatches entry ever existed for refreshRunningTargetWatches to start a stream from. But that protection was incidental: it depended on statement ordering inside a controller that nothing required anyone to preserve, with no test to catch a refactor that broke it. Move the decision to internal/authz.GitTargetAdmitted and apply it at every call site that compiles a rule or starts a data plane for a GitTarget: the GitTarget reconciler, the ClusterWatchRule reconciler, and the watch manager's startup bootstrap. The package is new because internal/controller imports internal/watch, so neither could host a helper the other calls. A refusal stops the data plane before it publishes status — the compiled rule is dropped and the watch manager replanned, then GitTargetNamespaceNotAuthorized and the terminal kstatus trio are written. A gate that only writes a condition leaves the stream running while announcing that it is not. A read error requeues rather than refusing, so a transient apiserver failure cannot tear down a running stream. Two new mappers (ClusterProvider -> ClusterWatchRules, Namespace -> ClusterWatchRules) make a revocation converge on the event rather than on the ~10 minute periodic reconcile; the GitTarget's own status flip cannot carry it, since GenerationChangedPredicate drops status-only updates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe change centralizes GitTarget namespace admission through ChangesNamespace admission enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterWatchRuleReconciler
participant authz.GitTargetAdmitted
participant KubernetesAPI
participant RuleStore
participant WatchManager
ClusterWatchRuleReconciler->>authz.GitTargetAdmitted: evaluate referenced GitTarget
authz.GitTargetAdmitted->>KubernetesAPI: read ClusterProvider and Namespace
authz.GitTargetAdmitted-->>ClusterWatchRuleReconciler: return admission decision
ClusterWatchRuleReconciler->>RuleStore: remove refused compiled rule
ClusterWatchRuleReconciler->>WatchManager: replan stream state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Implements PR 3 of the source-namespace addressing plan. Independent of the other four PRs. Bug fix — no API change, no CRD regeneration.
The defect
ClusterWatchRule.targetRefis aNamespacedTargetReferencewith a required namespace, and the reconciler resolved it with a plainr.Getand no authorization check of any kind. So a ClusterWatchRule could attach to a GitTarget in any namespace and widen that target's mirror scope cluster-wide, without the compilation path ever consulting the ClusterProvider'sallowedNamespacesadmission for that target's namespace.bootstrap.goseeded rules the same way.This was not a live escalation. ClusterWatchRule is cluster-scoped, so only a config-plane cluster-admin can create one, and that subject can already read the kubeconfig Secrets directly. The gate also held transitively:
checkSourceAuthorizationruns inside the Validated gate and returns beforeDeclareForGitTarget, which is what populatestargetWatches— andrefreshRunningTargetWatchesskips any table whose destination is not already running. An unauthorized rule built a resident table but started no stream.It is worth fixing anyway because that protection is incidental. It depends on statement ordering inside a controller that nothing requires anyone to preserve, with no test to catch a refactor of
DeclareForGitTargetthat silently converts it into a real hole. And it makesallowedNamespacesmean what it says: a platform admin reading a provider's admission list should be able to conclude that no rule anywhere mirrors through that credential on behalf of an unadmitted target.What changed
One shared decision.
internal/authz.GitTargetAdmittednow holds the provider-existence and namespace-admission check, applied by all three call sites that compile a rule or start a data plane for a GitTarget: the GitTarget reconciler, the ClusterWatchRule reconciler, and the watch manager's startup bootstrap. Bootstrap matters specifically because it runs before the first reconcile on every restart — a helper only the reconciler used would leave that whole window unguarded.A new package was necessary rather than stylistic:
internal/controllerimportsinternal/watch, so neither could host a helper the other calls without an import cycle.Refusal stops the data plane before it publishes status. The compiled rule is dropped and the watch manager replanned, and only then is
GitTargetNamespaceNotAuthorizedwritten with the terminal kstatus trio (Ready=False,Reconciling=False,Stalled=True). A gate that only writes a condition leaves the stream running while announcing that it is not.A read error requeues rather than refuses. A transient apiserver failure must not tear down a running stream, so the compiled rule is left in place and the error is returned.
Revocation converges on the event. Two new mappers — ClusterProvider → ClusterWatchRules and Namespace → ClusterWatchRules — cover the two ways a policy can change. The GitTarget's own status flip cannot carry this: it is a status-only update, which the existing
GenerationChangedPredicatedrops. Without the mappers a revocation would converge only on the ~10 minute periodic reconcile.The RBAC markers added to the ClusterWatchRule controller produce no manifest diff — both reads were already granted cluster-wide by the GitTarget controller's markers. They document this controller's actual reads.
Tests
internal/authzis at 100% statement coverage. Beyond the cases the plan lists, the table covers the deny-by-default shapes (nil policy, empty struct), the Names/Selector OR (a listed namespace is admitted even when the selector rejects it — guards a future refactor turning the OR into an AND), an invalid selector denying rather than admitting, and the absent-Namespace split (anamesentry still admits; a selector correctly does not).I verified the tests bite rather than trusting green. Three mutations against the merged code:
The third is the discrimination the plan explicitly asked for: it isolates ordering from end-state, and the other two tests correctly still pass under it because the final state is identical.
Two things worth your attention
1. I modified an existing test.
TestManagerStart_MustSeedRuleStoreFromExistingClusterWatchRuleshad a fixture whose GitTarget omitsclusterProviderRef(resolving todefault) with no ClusterProvider object present — so the new gate correctly refused it. I added the provider the wayinternal/controller/suite_test.goalready ships it for envtest ("the way a real install does", empty selector admitting every namespace). That is completing an incomplete fixture, not weakening a test: the refusal path it no longer exercises is now covered explicitly byTestBootstrapClusterWatchRule_RefusesMissingClusterProvider. Flagging it because altering a test to make a change pass deserves a second pair of eyes.2.
bootstrapWatchRulewas deliberately left ungated. A namespaced WatchRule's target lives in its own namespace, so that edge cannot cross a namespace boundary the way ClusterWatchRule's requiredtargetRef.namespacecan, and the GitTarget's own reconcile gate already governs whether that target's data plane runs. This matches the plan's scope, but it is a judgment call rather than a forced one.No e2e spec was added. Per the existing note that refused-GitTarget recovery is racy to assert end-to-end (the
GitPathAcceptedprojection is racy in both directions), a spec here would be a flake generator; the ordering contract is pinned deterministically at the unit level instead.Validation
task lint— 0 issuestask test— green; coverage ratchet auto-raised 76.7% → 77.0% (.coverage-baselinecommitted)task test-e2e— 58 passed, 0 failed, 22 skipped, 11m41s🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests